fix(withTableSettings): isSelected -> selected, isRequired -> required #1478

GermanVor · 2024-04-03T10:08:52Z

No description provided.

gravity-ui-bot · 2024-04-03T10:11:18Z

Playwright Test Component is ready.

gravity-ui-bot · 2024-04-03T10:11:21Z

Preview is ready.

amje

It breaks current behaviour

amje · 2024-04-03T10:30:47Z

src/components/Table/README.md

@@ -263,7 +263,7 @@ const MyTable1 = withTableSettings({sortable: false})(Table);
 ```ts
 type TableSettingsData = Array<{
  id: string;
-  isSelected?: boolean;
+  selected?: boolean;


Why this change? It's a breaking change

agree, need to separate TableColumnSetupItem from TableSetting and return backward compatibility for TableColumnSetupItem only

teleginzhenya · 2024-04-03T10:38:14Z

src/components/Table/hoc/withTableSettings/withTableSettings.tsx

@@ -23,26 +23,26 @@ import './withTableSettings.scss';

 export type TableSetting = {
    id: string;
-    isSelected?: boolean;
+    selected?: boolean;
 };

 export type TableSettingsData = TableSetting[];

 export type TableColumnSetupItem = TableSetting & {


Suggested change

export type TableColumnSetupItem = TableSetting & {

export type TableColumnSetupItem = {

teleginzhenya · 2024-04-03T10:38:37Z

src/components/Table/hoc/withTableSettings/withTableSettings.tsx

+    required?: boolean;
 };


Suggested change

required?: boolean;

};

selected?: boolean;

required?: boolean;

};

teleginzhenya · 2024-04-03T10:39:36Z

src/components/Table/hoc/withTableSettings/withTableSettings.tsx

@@ -23,26 +23,26 @@ import './withTableSettings.scss';

 export type TableSetting = {
    id: string;
-    isSelected?: boolean;
+    selected?: boolean;


Suggested change

selected?: boolean;

isSelected?: boolean;

GermanVor · 2024-04-04T11:15:21Z

The problem is real

I rewrote TableColumnSetup but I did not think that it was public component. Interfaces TableColumnSetupItem also is public, but it is not connected with the TableSetting.

So, the best solution now to save backward compatibility in my opinion

stop exporting new TableColumnSetup
create separated TableColumnSetup component with code before I rewrote TableColumnSetup and export it

3616e97
645df56

What to do with new created TableColumnSetup component I do not know - probably delete it in future, but now save it for backward compatibility.

@teleginzhenya @Raubzeug

teleginzhenya · 2024-04-04T14:21:12Z

@amje @korvin89 @Raubzeug @ValeraS we've discussed it and decided to return backward compatibility of public api of TableColumnSetup component for now. Then we'll discuss next steps about this component, should it be exported as separate component or not.

GermanVor · 2024-04-05T12:05:36Z

@amje @korvin89 @Raubzeug @ValeraS

I begin rewrite new TableColumnSetup with the props of old one and I have new thoughts:

If I rewrite a new component (TableColumnSetup with TreeSelect) for the old ones, then I risk breaking the new component and the old component (TableColumnSetup with List).
The old component had props, which it is unclear how to apply to the new component (for example filter - there is no such property in TreeSelect)
The behavior of the old component is slightly different from the new one - in the new component we allow the required columns to be moved, but we do not allow to hide them, while in old component case we moved required columns at the start of the list, without opportunity move them.
The old component had the className option - it is not difficult to return it, but if the users used complex selectors, they are very likely will be broken cause TreeSelect probably is not comparable with List and Popup.

The best way to save backward compatibility in my opinion will be create TableColumnSetup with old code (code before i rewrote TableColumnSetup with TreeSelect) and export it instead of TableColumnSetup from withTableSettings.

In this case we 100% sure that new component will not be broken, users that use TableColumnSetup will get back component that they expected, old component 100% will not be broken.

And after that we will think what to do with TableColumnSetup.

Raubzeug · 2024-04-05T12:16:38Z

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

@@ -18,7 +18,7 @@ import type {
 import type {ListItemViewProps} from '../../../../useList';
 import {ListContainerView, ListItemView} from '../../../../useList';
 import {block} from '../../../../utils/cn';
-import type {TableColumnSetupItem, TableSetting} from '../withTableSettings';
+import type {TableSetting} from '../withTableSettings';


We have a circular dependency here, let's avoid such a case.

Raubzeug · 2024-04-05T12:17:03Z

src/components/TableColumnSetup/index.ts

@@ -0,0 +1 @@
+export * from './TableColumnSetup';


Should export it also in https://github.com/gravity-ui/uikit/blob/645df5615cc52f419cd7cb57865af454ba1ad8be/src/components/index.ts

GermanVor · 2024-04-06T01:50:16Z

src/components/Table/__tests__/Table.withTableSettings.test.tsx

@@ -288,7 +288,7 @@ describe('withTableSettings', () => {

            await userEvent.click(screen.getByRole('button', {name: 'Table settings'}));
            await userEvent.click(await screen.findByRole('button', {name: 'description'}));
-            await userEvent.click(screen.getByRole('button', {name: 'Apply'}));
+            await userEvent.click(screen.getByRole('button', {name: 'button_apply'}));


have no idea why it is broken now

This is because you test component const TableWithSettings = withTableSettings<SomeItem>(Table); . And keysets are taken from https://github.com/gravity-ui/uikit/tree/839d503f378e23eef87887866d20973bcd174fb4/src/components/Table/hoc/withTableSettings/i18n

Will it be fixed?

In separate PR, I do not know what happened with this place

GermanVor · 2024-04-06T01:53:31Z

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

+    let lastStickyLeftIdx = 0;
+    for (; lastStickyLeftIdx !== visibleFlattenIds.length; lastStickyLeftIdx++) {
+        const visibleFlattenId = visibleFlattenIds[lastStickyLeftIdx];
+        const item = items.find((item) => item.id === visibleFlattenId) as Item | undefined;


@IsaevAlexandr ListItemType generic works bad.
Do not know why.

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

ValeraS · 2024-04-06T12:17:29Z

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

            startSlot: tableColumnItem.isRequired ? <Icon data={Lock} /> : undefined,
            hasSelectionIcon,
            selected: hasSelectionIcon ? tableColumnItem.isSelected : undefined,
        };
    });
 };

+const prepareStikyState = (items: ListItemType<Item>[], visibleFlattenIds: string[]) => {
+    let lastStickyLeftIdx = 0;


left and right names are obsolete in our code base, please use start and end instead.

ValeraS · 2024-04-06T12:20:08Z

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

+    const stickyLeftItemIdList: string[] = [];
+    for (let i = 0; i < lastStickyLeftIdx; i++) {
+        const visibleFlattenId = visibleFlattenIds[i];
+        stickyLeftItemIdList.push(visibleFlattenId);
+    }
+
+    const sortableItemIdList: string[] = [];
+    for (let i = lastStickyLeftIdx; i < firstStickyRightIdx; i++) {
+        const visibleFlattenId = visibleFlattenIds[i];
+        sortableItemIdList.push(visibleFlattenId);
+    }
+
+    const stickyRightItemIdList: string[] = [];
+    for (let i = firstStickyRightIdx; i < visibleFlattenIds.length; i++) {
+        const visibleFlattenId = visibleFlattenIds[i];
+        stickyRightItemIdList.push(visibleFlattenId);
+    }


Suggested change

const stickyLeftItemIdList: string[] = [];

for (let i = 0; i < lastStickyLeftIdx; i++) {

const visibleFlattenId = visibleFlattenIds[i];

stickyLeftItemIdList.push(visibleFlattenId);

}

const sortableItemIdList: string[] = [];

for (let i = lastStickyLeftIdx; i < firstStickyRightIdx; i++) {

const visibleFlattenId = visibleFlattenIds[i];

sortableItemIdList.push(visibleFlattenId);

}

const stickyRightItemIdList: string[] = [];

for (let i = firstStickyRightIdx; i < visibleFlattenIds.length; i++) {

const visibleFlattenId = visibleFlattenIds[i];

stickyRightItemIdList.push(visibleFlattenId);

}

const stickyLeftItemIdList = visibleFlattenIds.slice(0, lastStickyLeftIdx);

const sortableItemIdList = visibleFlattenIds.slice(lastStickyLeftIdx, firstStickyRightIdx);

const stickyRightItemIdList = visibleFlattenIds.slice(firstStickyRightIdx);

ValeraS · 2024-04-06T12:38:12Z

src/components/TableColumnSetup/TableColumnSetup.tsx

+    const items = propsItems.map(({id, title, required, selected}) => ({
+        id,
+        title,
+        isRequired: required,
+        isSelected: selected,
+    }));


Suggested change

const items = propsItems.map(({id, title, required, selected}) => ({

id,

title,

isRequired: required,

isSelected: selected,

}));

const items = propsItems.map(({id, title, required, selected}) => ({

id,

title,

isRequired: required,

isSelected: selected,

sticky: required ? "start" : undefiend

}));

or add a sticky field to TableColumnSetupItem, otherwise the behavior of the component will be different from the previous one.

ValeraS · 2024-04-07T21:21:22Z

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

@@ -302,7 +365,7 @@ export const TableColumnSetup = (props: TableColumnSetupProps) => {

    return (
        <TreeSelect
-            className={tableColumnSetupCn}
+            className={b(null, className)}
            mapItemDataToProps={identity}


to get rid of these warnings and not set unknown attributes on DOM elements

let's do next

Suggested change

mapItemDataToProps={identity}

mapItemDataToProps={({isRequired, isSelected, sticky, ...rest}) => rest}

and remove ...data from the commonProps in the renderDndItem function.

or better instead of prepareDndItems and dndItems use mapItemToProps and items

Suggested change

mapItemDataToProps={identity}

mapItemDataToProps={mapItemToProps}

items={items}

where

function mapItemToProps(item: TableColumnSetupItem): ListItemCommonProps { return { id: item.id, title: item.title, startSlot: item.isRequired ? <Icon data={Lock} /> : undefined, hasSelectionIcon: !item.isRequired, selected: !item.isRequired ? tableColumnItem.isSelected : undefined, }; }

I think main reason is here

ValeraS · 2024-04-07T21:21:26Z

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

+    isRequired?: boolean;
+    sticky?: TableColumnConfig<unknown>['sticky'];
+};
+
 type Item = TableColumnSetupItem &


Why is {id, isDragDisabled} needed? id is present in TableSetting and isDragDisabled is not used.

Raubzeug · 2024-04-08T07:44:59Z

src/components/Table/__tests__/Table.withTableSettings.test.tsx

@@ -288,7 +288,7 @@ describe('withTableSettings', () => {

            await userEvent.click(screen.getByRole('button', {name: 'Table settings'}));
            await userEvent.click(await screen.findByRole('button', {name: 'description'}));
-            await userEvent.click(screen.getByRole('button', {name: 'Apply'}));
+            await userEvent.click(screen.getByRole('button', {name: 'button_apply'}));


This is because you test component const TableWithSettings = withTableSettings<SomeItem>(Table); . And keysets are taken from https://github.com/gravity-ui/uikit/tree/839d503f378e23eef87887866d20973bcd174fb4/src/components/Table/hoc/withTableSettings/i18n

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

amje · 2024-04-08T13:56:08Z

src/components/Table/__tests__/Table.withTableSettings.test.tsx

@@ -288,7 +288,7 @@ describe('withTableSettings', () => {

            await userEvent.click(screen.getByRole('button', {name: 'Table settings'}));
            await userEvent.click(await screen.findByRole('button', {name: 'description'}));
-            await userEvent.click(screen.getByRole('button', {name: 'Apply'}));
+            await userEvent.click(screen.getByRole('button', {name: 'button_apply'}));


Will it be fixed?

amje · 2024-04-08T13:58:50Z

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx

-const prepareDndItems = (tableColumnItems: TableColumnSetupItem[]) => {
-    return tableColumnItems.map<Item>((tableColumnItem) => {
-        const hasSelectionIcon = tableColumnItem.isRequired === false;
+const prepareStikyState = (


Suggested change

const prepareStikyState = (

const prepareStickyState = (

amje · 2024-04-08T14:09:38Z

src/components/TableColumnSetup/TableColumnSetup.tsx

@@ -0,0 +1,130 @@
+import React from 'react';


Why we can't just move the component outside the Table?

it is only to save backward compatibility with old TableColumnSetup, TableColumnSetup from Table is not exportable.

Let's simply move it up to the components. It should be the one component. Keep backward compatibility with old versions and add sticky feature

amje · 2024-04-08T15:08:22Z

src/components/TableColumnSetup/TableColumnSetup.tsx

@@ -0,0 +1,130 @@
+import React from 'react';


Let's simply move it up to the components. It should be the one component. Keep backward compatibility with old versions and add sticky feature

GermanVor requested a review from Raubzeug as a code owner April 3, 2024 10:08

korvin89 changed the title ~~fix(withTbleSettings): isSelected -> selected~~ fix(withTbleSettings): isSelected -> selected, isRequired -> required Apr 3, 2024

korvin89 approved these changes Apr 3, 2024

View reviewed changes

teleginzhenya self-requested a review April 3, 2024 10:19

teleginzhenya approved these changes Apr 3, 2024

View reviewed changes

amje requested changes Apr 3, 2024

View reviewed changes

teleginzhenya self-requested a review April 3, 2024 10:36

teleginzhenya requested changes Apr 3, 2024

View reviewed changes

GermanVor force-pushed the withTableSettings-TableColumnSetupItem branch from 6dce2a5 to 645df56 Compare April 4, 2024 11:05

GermanVor requested a review from ValeraS as a code owner April 4, 2024 11:05

Raubzeug requested changes Apr 5, 2024

View reviewed changes

GermanVor force-pushed the withTableSettings-TableColumnSetupItem branch 2 times, most recently from 2b05fc1 to 2c4bb9a Compare April 6, 2024 01:01

GermanVor commented Apr 6, 2024

View reviewed changes

ValeraS requested changes Apr 6, 2024

View reviewed changes

GermanVor force-pushed the withTableSettings-TableColumnSetupItem branch from de23fe0 to 839d503 Compare April 6, 2024 22:29

ValeraS requested changes Apr 8, 2024

View reviewed changes

Raubzeug requested changes Apr 8, 2024

View reviewed changes

ValeraS requested changes Apr 8, 2024

View reviewed changes

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx Outdated Show resolved Hide resolved

src/components/Table/hoc/withTableSettings/TableColumnSetup/TableColumnSetup.tsx Show resolved Hide resolved

ValeraS approved these changes Apr 8, 2024

View reviewed changes

Raubzeug approved these changes Apr 8, 2024

View reviewed changes

amje requested changes Apr 8, 2024

View reviewed changes

GermanVor added 2 commits April 8, 2024 16:28

chore: remove TableColumnSetupItem from export

3b1785c

feat: new tableColumnSetup with old ableColumnSetupProps

4aec3d3

GermanVor added 13 commits April 8, 2024 16:28

feat: add sticky support for withTableSettings

d677d5a

chore: new tableColumnSetup with old ableColumnSetupProps

458d812

chore: absolute import path

3a68f6c

chore: fix test

7685c37

chore: itemsById instead items.find

5c2df09

chore: unknown

3e0caca

chore: obsolete words

260152e

chore: remove fors

2c18968

chore: add sticky for old TableColumnSetup

ace54b3

chore: remove isDragDisabled from Item

a97ab2d

chore: remove warnings

be995eb

chore: remove prepareDndItems

d309b2d

chore: prepareStickyState

79d33de

GermanVor force-pushed the withTableSettings-TableColumnSetupItem branch from 662224f to 79d33de Compare April 8, 2024 14:29

amje requested changes Apr 8, 2024

View reviewed changes

Merge branch 'main' into withTableSettings-TableColumnSetupItem

1c17566

amje changed the title ~~fix(withTbleSettings): isSelected -> selected, isRequired -> required~~ fix(withTableSettings): isSelected -> selected, isRequired -> required Apr 9, 2024

Merge branch 'main' into withTableSettings-TableColumnSetupItem

856c4c0

amje approved these changes Apr 9, 2024

View reviewed changes

amje merged commit cf1dfb0 into main Apr 9, 2024
4 checks passed

amje deleted the withTableSettings-TableColumnSetupItem branch April 9, 2024 15:17

gravity-ui-bot mentioned this pull request Apr 9, 2024

chore(main): release 6.10.0 #1482

Merged

ValeraS mentioned this pull request Apr 12, 2024

Bug(withTableSettings): redundant props in withSettingsTable which shouldn't be in DOM attributes #1449

Closed

	export type TableColumnSetupItem = TableSetting & {
	export type TableColumnSetupItem = {

	mapItemDataToProps={identity}
	mapItemDataToProps={({isRequired, isSelected, sticky, ...rest}) => rest}

	mapItemDataToProps={identity}
	mapItemDataToProps={mapItemToProps}
	items={items}

fix(withTableSettings): isSelected -> selected, isRequired -> required #1478

fix(withTableSettings): isSelected -> selected, isRequired -> required #1478

Conversation

GermanVor commented Apr 3, 2024

gravity-ui-bot commented Apr 3, 2024

gravity-ui-bot commented Apr 3, 2024

amje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teleginzhenya Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GermanVor commented Apr 4, 2024 • edited Loading

teleginzhenya commented Apr 4, 2024

GermanVor commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValeraS Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teleginzhenya Apr 3, 2024 •

edited

Loading

GermanVor commented Apr 4, 2024 •

edited

Loading

ValeraS Apr 6, 2024 •

edited

Loading